Would it work (and be simpler) to just use Buffer.byteLength() here and here?

Ref: #1901

caculate byte length maybe make performance slow. Would like to hear more suggestion.

Ditto on the byteLength() call. That's the (or at least should be) "standard" way.

Thanks @cjihrig @trevnorris . Use byteLength now.

@cjihrig updated.

ping @nodejs/collaborators

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/778/

LGTM

CI is not happy.

oops. err not found in windows....
https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/778/nodes=win2008r2/tapTestReport/test.tap-23/

the windows doesn't like unicode. hmmm.

I think maybe reached an edge case for windows platform.

Anyone can help me to restart the CI?

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/781/

The test failures seem to be related to a known issue on Windows - nodejs/node-v0.x-archive#7940.

@piscisaureus is this something that can be skipped on Windows during testing?

can we skip the test case for win32? ping @piscisaureus

You can probably skip the test case with something like the crypto skip, but with common.isWindows.

anyone can help me to restart the CI?

I think what @piscisaureus is getting at is that partial character sequences throw off the length calculation; the .setEncoding(encoding) call makes the stream convert bytes to characters and incomplete multi-byte sequences get buffered by the StringDecoder until more data is available (which may never come so it's imprecise at best.)

Apart from that stipulation the patch looks correct to me. It's rather inefficient though: we first convert from bytes to characters and then we convert that back to bytes for the length calculation.

@bnoordhuis We can don't set encoding, so don't use StringDecoder, just only encode buffer before execute callback.

I think you will still need a StringDecoder instance to deal with partial character sequences. If you encode through just buf.toString(encoding), it's either going to throw an exception or lose bytes.

I don't think we can just remove the .setEncoding() calls; that would almost certainly break existing code. Perhaps it's possible to add a fast path to stream.Readable, presumably in readableAddChunk(), although that code is already a twisty little maze of if statements.

With the commit bfb2cd0 be landed, the PR can be implemented more light-weight.

LGTM. This should hopefully avoid any encoding issues.

@bnoordhuis @piscisaureus @trevnorris thoughts?

Would it be possible to get another round of reviews from some combination of @bnoordhuis @piscisaureus and @trevnorris ? It would be nice to see this finally land if the objections have been sufficiently addressed. (And if not, it would be nice to have the objections documented or reiterated.)

Nit: Can you add Fixes: https://github.com/nodejs/node/issues/1901 metadata line to the commit message?

Nit: Can you find a way to shorten the first line of the commit message so that it is 50 characters or less?

For the test would it be possible to spawn a child node process and write to its stderr/stdout? That should work for all platforms.

It looks like the test still needs an update.

bfb2cd0 was reverted, this issue needs hold.

I believe this can be closed now given that #6764 landed.

thanks @jasnell @Trott